Make restricted room join tests clearer #752
Conversation
…o strict ruma parsing requirements Signed-off-by: strawberry <[email protected]>
Signed-off-by: strawberry <[email protected]>
Signed-off-by: morguldir <[email protected]>
| // This should be ignored since this is a join -> join transition. | ||
| "join_authorised_via_users_server": "unused", | ||
| // This should be ignored by the server since this is a join -> join transition | ||
| "join_authorised_via_users_server": "@unused:unused.local", |
There was a problem hiding this comment.
What happens if a ruma-based server gets garbage here then? Does it still work?
There was a problem hiding this comment.
It can create the event then, but it won’t really work properly unless the server removes the field when handling the PUT like synapse does
So when verifying the signature of a federation event actually looking like that it won’t work still
This part i think is intended though, although seemingly synapse changed that behaviour at some point too 🧐
There was a problem hiding this comment.
I'm not really following your comment, sorry. Shouldn't the field be removed no matter what? So what isn't working for ruma?
There was a problem hiding this comment.
Sorry yeah it should, but if another server forgets to remove it, then ruma still tries to check the signature, but synapse seems to still allow the event
There was a problem hiding this comment.
Irrespective of the behaviour of Synapse and Ruma, this field should either be set to a valid structure for this field (a Matrix ID), or it should explicitly call out that this field is set to an invalid value on purpose.
As the latter does not appear the case, I agree with this change. However I'm not opposed to another test that checks homeservers correctly handle invalid content in this field.
|
|
||
| return ev.Get("content").Get("membership").Str == "leave" | ||
| })) | ||
|
|
There was a problem hiding this comment.
This is a duplicate of the stuff underneath? Why add this?
There was a problem hiding this comment.
It makes sure the leave worked in both rooms now
There was a problem hiding this comment.
Wouldn't this break if a single sync contained both leaves?
MustSyncUntil supports multiple SyncCheckOpt instances, so you can provide two functions that verify that bob has left the rooms in question.
You can also replace SyncTimelineHas with SyncLeftFrom to save yourself the manual event content checking.
|
|
||
| return ev.Get("content").Get("membership").Str == "leave" | ||
| })) | ||
|
|
There was a problem hiding this comment.
Wouldn't this break if a single sync contained both leaves?
MustSyncUntil supports multiple SyncCheckOpt instances, so you can provide two functions that verify that bob has left the rooms in question.
You can also replace SyncTimelineHas with SyncLeftFrom to save yourself the manual event content checking.
| // This should be ignored since this is a join -> join transition. | ||
| "join_authorised_via_users_server": "unused", | ||
| // This should be ignored by the server since this is a join -> join transition | ||
| "join_authorised_via_users_server": "@unused:unused.local", |
There was a problem hiding this comment.
Irrespective of the behaviour of Synapse and Ruma, this field should either be set to a valid structure for this field (a Matrix ID), or it should explicitly call out that this field is set to an invalid value on purpose.
As the latter does not appear the case, I agree with this change. However I'm not opposed to another test that checks homeservers correctly handle invalid content in this field.
Firstly this PR makes the tests work on more implementations, for example ruma and GMS won't really understand
join_authorised_via_users_servernot looking like a user id, similar to what #224 tried to doMaking it a user id that isn't correct still makes sure that the server can handle clients updating the member event directly with /myroomnick or similar
Some errors were also silently being ignored, which made it harder to tell which part of the test was actually failing
Pull Request Checklist